-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
contracts & client config: scale building cost #1722
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Failed to generate code suggestions for PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of scaled building costs looks good and covers both contract and client-side changes. The new BuildingGeneralConfig
model and its usage are well-implemented. The changes to the create
function in BuildingCustomImpl
to return both the Building
and BuildingQuantityv2
are appropriate. The client-side SDK updates in EternumProvider
and config/index.ts
are consistent with the contract changes. The removal of the BonusPercentageImpl
in favor of using PercentageImpl
and PercentageValueImpl
from the math utils is a good refactoring. The changes seem to be backwards compatible and shouldn't break existing functionality. Overall, this is a solid implementation of the scaled building costs feature.
Thanks for using MentatBot. Give comments a 👍 or 👎 to help me improve!
@@ -102,7 +102,7 @@ export const ONE_MONTH = 2628000; | |||
|
|||
// Buildings | |||
export const BASE_POPULATION_CAPACITY = 5; | |||
|
|||
export const BUILDING_FIXED_COST_SCALE_PERCENT = 1_000; // 1_000/10_000 = 10% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making this scaling factor more easily configurable, possibly as an environment variable or part of a configuration object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
contracts and client config for #1720